-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP Add Rational RingSolver #2215
base: master
Are you sure you want to change the base?
Conversation
Thanks, looks great! As you say if you could add some examples in |
f : (x : ℚ) → Maybe (0ℚ ≡ x) | ||
f (mkℚ (pos 0) 0 _) = just refl | ||
f (mkℚ (pos 0) (suc _) _) = nothing | ||
f (mkℚ (pos (suc _)) _ _) = nothing | ||
f (mkℚ (negsuc _) _ _) = nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be strongly tempted to lift this out as a definition in Data.Rational.Base
, or else as a property (cf. Relation.Binary.WeaklyDecidable
, for which we currently lack analogues at arity 0, 1) in Data.Rational.Properties
.
Suggested name for such a refactored f
: isZero-weakly-decidable
?
That way, the import
s for your solver modules then become very much simplified, in favour of those that are already currently used by the Rational.*
modules...
... and there's (potentially) a downstream benefit in being able to reuse the definition of the corresponding Unnormalised
function in the definition of this one.
where | ||
f : (x : ℚᵘ) → Maybe (0ℚᵘ ≃ x) | ||
f (mkℚᵘ (pos zero) _) = just (*≡* refl) | ||
f (mkℚᵘ (pos (suc _)) _) = nothing | ||
f (mkℚᵘ (negsuc _) _) = nothing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.!
Re: refactoring suggestion I think that it's possible that a 'better' approach is available via leveraging the construction in the (old) Now, equality on |
Re: refactoring (meta-comment) I think there's a tension, especially as a new developer, between contributing a PR with as 'small' footprint as possible (which can have as a consequence a 'wide' collection of For the PR at hand: The definitions of the |
There is clearly something wrong with RingSolvers in this PR. I defined very simple example here:
but this fails with: Malformed call to solve.Expected target type to be like: ∀ x y → x + y ≈ y + x.Instead: _19
when checking that the expression unquote solve-∀ has type _19 I think RingSolver does not like PropositionalEquality (but it does not like equality define in Rational.Properties either). I will try to see if I can create example with existing SemiringSolver for begin with and do refactoring proposed, by @jamesmckinna. |
Ok, so this is clearly very troubling behaviour! @oisdk do you have any insight here to offer? |
Given that the goal looks like a meta variable, it may be that the tactic is being Cf. the |
I've found some resources on semigroup/ring solvers in Agda:
I will try to traverse the subject and fold it into better understanding - what is the problem here. |
Changes
Fix #1879